feat(cli): port supabase inspect report to native TypeScript#5565
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase@5565Preview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2850b07d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // plain column reference | ||
| const col = this.parseColRef(); | ||
| return { column: col }; |
There was a problem hiding this comment.
Preserve csvq expressions in custom rules
When a project has custom [experimental.inspect.rules] using normal csvq SELECT expressions (for example SELECT COALESCE(name, '-') FROM ... or other functions/operators), the Go implementation passes r.Query directly to the csvq driver and scans the first column, but this parser only accepts an aggregate call or a bare column reference. Those valid custom rules now render a parser error in the STATUS cell instead of evaluating to pass/fail, so existing report rules outside the built-in subset regress.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The parity description is accurate: Go hands r.Query straight to the real csvq driver (apps/cli-go/internal/inspect/report.go), so a custom rule using full csvq syntax — COALESCE, CASE, joins, subqueries, etc. — evaluates to a real pass/fail there. This evaluator implements only a bounded subset (SELECT <agg|bare-column> FROM \csv` [WHERE ...]), so an out-of-subset custom rule surfaces a parser-error STATUS cell instead of a result. The 7 built-in default rules (templates/rules.toml`) are all inside the supported grammar and test-covered, so there is no regression for them; custom rules outside the subset do diverge from Go.
This is a deliberate, documented limitation (see the header of report.csvq.ts): there is no JS/TS port of mithrandie/csvq, and neither DuckDB nor alasql accepts csvq's dialect, so closing the gap fully means reimplementing the csvq expression dialect in TS — a large piece of work, not a quick follow-up. Flagging this as a scope decision for a maintainer rather than silently absorbing the divergence or pulling a full csvq reimplementation into this PR; leaving the thread open. Options:
- (a) accept the bounded subset as the documented limit and track full csvq support as a follow-up issue;
- (b) detect out-of-subset queries and emit a clearer "unsupported csvq expression" STATUS rather than a raw parser error;
- (c) treat full custom-rule parity as in-scope here (large reimplementation; own PR).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32524fe2eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const chunks: Array<Buffer> = []; | ||
| stream.on("data", (chunk: Buffer) => chunks.push(chunk)); | ||
| stream.on("error", (error: Error) => | ||
| resume( | ||
| Effect.fail(new LegacyDbCopyError({ message: `failed to copy output: ${error}` })), | ||
| ), | ||
| ); | ||
| stream.on("end", () => resume(Effect.succeed(new Uint8Array(Buffer.concat(chunks))))); |
There was a problem hiding this comment.
Stream COPY output instead of buffering it
For databases with many tables/indexes, report CSVs such as table or index stats can become large, but this accumulates every COPY chunk in memory, concatenates the full buffer, and the caller then keeps those bytes for rule evaluation. The Go implementation streams CopyTo directly into the output file, so this port can exhaust the Bun process heap or stall on a large report where the Go CLI would complete with bounded memory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right that Go streams CopyTo straight into the file (internal/inspect/report.go:64-77) and re-reads the CSVs from disk for the rules check (sql.Open("csvq", outDir), report.go:97), so its peak memory is bounded, whereas this port buffers each COPY via Buffer.concat and pins the CSVs in an in-memory map for rule evaluation.
This is purely a resource/robustness difference, not an output one — the CSV bytes, the rules table, and the exit code are byte-identical to Go on every observable dimension (the server serializes the CSV; the TS side never touches the values), and the report runs a fixed set of catalog-stat queries rather than streaming row data, so the OOM risk is confined to pathologically large databases. Matching Go's streaming means reshaping the copyToCsv driver seam (take an output path / return a write sink) and switching the rule evaluator to read CSVs back from disk — and that seam has already changed a couple of times this review cycle. I'd rather land the byte-identical behavior now and do the streaming rework as a focused follow-up with its own tests; tracking it rather than folding it in here. Leaving this thread open for that follow-up.
Promotes the last `inspect` leaf from a Phase 0 Go proxy to a native TS port (CLI-1317). The command runs every inspect query via server-side `COPY (...) TO STDOUT WITH CSV HEADER`, writes 14 CSVs under `<output-dir>/<YYYY-MM-DD>/`, then renders a Go-parity Glamour rules summary table validating those CSVs. - Extend the shared `LegacyDbConnection` driver with `copyToCsv`, which reuses a single raw `pg` connection (via `pg-copy-streams`) for all copies against the winning dial target, matching Go's single `pgconn`. - Add a bounded csvq-subset evaluator (`report.csvq.ts`) replicating csvq value/type-comparison semantics for the inspect rule grammar. - Support custom `[experimental.inspect.rules]` from config.toml with `env(VAR)` expansion; default to the 7 embedded rules otherwise. - Hoist the shared inspect base layer into `inspect/inspect.layers.ts`. - Add `legacy/output/legacy-bold.ts` (lipgloss Bold parity). - json/stream-json output modes are a TS-only addition (Go is text-only).
Promoting `inspect report` from a Go proxy to a native TS handler exposed two e2e mock gaps (the parity tests previously compared Go-vs-Go and passed trivially): - The native driver eagerly forces its lazy `pg` connection with a simple `SELECT 1` probe; the pg-mock rejected it in the empty state, so the command failed to connect. A real Postgres always answers `SELECT 1` — make the mock do so too, regardless of fixture state. - The rules summary is computed by csvq over COPY CSV content the mock cannot emit (it returns an empty, header-less COPY for every query). On those empty CSVs Go's csvq panics with an "index out of range" fatal error printed into each STATUS cell, while the native TS evaluator reports a clean "unknown column" — so stdout is not faithfully comparable. Add a documented `compareStdout` opt-out to runParity and use it for the report parity test; exit code, stderr, request log, and the 14 written CSVs are still compared, and the rules-table rendering is covered by the apps/cli unit + integration tests against real fixtures.
…ules Go loads `[experimental.inspect].rules` via `viper.UnmarshalExact` without disabling `WeaklyTypedInput` (config.go:579-584), so mapstructure coerces scalar rule fields to strings (123 → "123", true → "1") and aborts only when an array entry is not a table or a field is a non-coercible type. The TS reader was coercing non-strings to "" and silently skipping non-table entries, so a broken custom rule could replace the defaults and let the command succeed with empty rules. Coerce scalar fields the same way Go does, and fail with LegacyDbConfigLoadError on a non-table entry or a non-coercible field — matching Go's "expected a map or struct" / "expected type string" decode errors. (review: report.config rule validation)
Three parity gaps in how custom `[experimental.inspect.rules]` are loaded, all
grounded in Go's `UnmarshalExact` (config.go:579) under viper's default
`WeaklyTypedInput: true` and its `PersistentPreRunE` config load:
- Validate the rule config BEFORE connecting or writing CSVs. Go loads/validates
the whole config in PersistentPreRunE (via ParseDatabaseConfig → LoadConfig),
so a malformed config aborts with zero side effects. The reader now runs at the
top of the handler, before mkdir/connect/COPY (rules are still applied in the
summary step). (review: validate-before-COPY)
- Reject a non-array `rules` value the way Go's decodeSlice does under weak
typing: a scalar (`rules = "foo"`) aborts ("expected a map or struct"); a single
inline table is wrapped into one rule; an empty table yields no custom rules.
Previously any non-array silently fell through to the defaults. (review: non-array rules)
- Reject unknown/misspelled keys in a rule table. UnmarshalExact sets mapstructure
ErrorUnused per-struct, so `fails = "bad"` aborts the load; the reader now fails
with LegacyDbConfigLoadError instead of silently ignoring extra keys. (review: unknown keys)
Adds unit coverage (unknown key, single inline table, scalar rules) and an
integration test asserting a malformed config aborts before any connection or CSV.
23a598e to
7f9077f
Compare
jgoux
left a comment
There was a problem hiding this comment.
Reviewed PR head 23a598ec.
Findings:
- P2: COPY runs before confirming the CSV file can be written
In apps/cli/src/legacy/commands/inspect/report/report.handler.ts, the handler calls session.copyToCsv(...) before attempting to write the destination CSV with fs.writeFile(...). Go opens/truncates the destination file first, then executes CopyTo (apps/cli-go/internal/inspect/report.go:66-73).
That changes failure side effects: with a bad output target, permissions issue, or file-as-directory path, the TS path can still run remote inspection queries before discovering that the report cannot be written, while Go fails before touching the database. This is separate from the existing buffering concern, though streaming directly to the file would naturally fix both.
- P2: invalid
[experimental.inspect]parent shapes/keys still fall through to defaults
The earlier review fixes cover malformed rules entries, scalar rules, and unknown keys inside rule tables. However, report.config.ts still treats experimental.inspect itself as optional via asRecord(...) and falls back to default rules when rules is absent.
Examples like [experimental.inspect] rulez = [...] or inspect = "oops" should fail under Go's exact config decoding: Go's inspect struct only has rules (apps/cli-go/pkg/config/config.go:236-238) and the config is loaded with UnmarshalExact (config.go:579). The TS reader should validate the parent object shape and reject unexpected keys there too, rather than silently running the default report rules.
Notes on existing threads:
- The comments about non-string fields, non-array values, late validation, and unknown rule-table keys look addressed in
23a598ec. - The bounded csvq subset and buffered COPY output remain real behavior/resource tradeoffs. They may be explicit scope decisions, but they are still material differences from Go for custom rules and large reports.
Verification run locally:
- report unit tests: 58 passed
- report integration test: 22 passed
- shared DB connection unit test: 26 passed
nx run supabase:types:checkpassed
The rebase conflict resolution left @types/pg and @types/pg-copy-streams snapshots pinned to @types/node@25.9.1 while develop bumped to 25.9.3, breaking CI's --frozen-lockfile install. Normalize both edges to 25.9.3.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cefcc5fa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fresh = new Pg.Client(winningRawConfig); | ||
| yield* Effect.tryPromise({ | ||
| try: () => fresh.connect(), |
There was a problem hiding this comment.
Avoid opening a second connection for COPY
When the target role/database has only one free connection slot (for example a CONNECTION LIMIT 1 role or a saturated max_connections), connect already holds the probed PgClient, and the first copyToCsv then dials this additional pg.Client. The Go report path reuses the single conn.PgConn() returned by ConnectByConfig for CopyTo, so it can succeed in that environment while this port fails before writing the report. Please run COPY on the existing/raw connection or release the probe connection before opening the COPY client.
Useful? React with 👍 / 👎.
| if (rawRules === undefined) { | ||
| return [] as ReadonlyArray<LegacyInspectRule>; |
There was a problem hiding this comment.
Reject invalid inspect config instead of defaulting
When config.toml has an invalid inspect section with no valid rules key, such as [experimental.inspect] ruless = [...] or inspect = "bad", this treats the rules as absent and runs the default checks. Go loads the whole config with UnmarshalExact into Experimental.Inspect, so unknown fields or the wrong shape under experimental.inspect abort before any report queries; silently defaulting hides misspelled custom rules. Please validate the inspect section's shape and keys before returning [].
Useful? React with 👍 / 👎.
| let inQuotes = false; | ||
| let started = false; | ||
| const pushField = () => { | ||
| row.push(field); |
There was a problem hiding this comment.
Preserve CSV nulls when evaluating rules
When a report CSV contains an unquoted empty field from a SQL NULL (for example db_stats.index_hit_rate can be NULL when the hit-rate denominator is zero), csvq imports that as NULL, but this parser collapses it to the same "" value as a quoted empty string. The default cache-hit rule can then string-compare "" < 0.94 and report a failure where Go's csvq evaluates NULL < 0.94 as unknown and excludes the row; custom rules using IS NULL, COUNT(col), or aggregates over nullable columns drift for the same reason. Please track whether a field was quoted/unquoted and model unquoted empty fields as NULL.
Useful? React with 👍 / 👎.
| const inspect = asRecord(asRecord(doc?.["experimental"])?.["inspect"]); | ||
| const rawRules = inspect?.["rules"]; |
There was a problem hiding this comment.
Apply matching remote inspect rules
When the linked project matches a [remotes.<name>] block that defines its own [remotes.<name>.experimental.inspect.rules], Go resolves the project ref before LoadConfig and merges that remote block before printSummary, but this reader only looks at the top-level experimental.inspect table. Those remote-specific report rules are ignored and the command falls back to the top-level/default rules for the linked project, so multi-environment configs validate the wrong policy. Please merge the matching remote config or defer rule loading until after the linked ref is known.
Useful? React with 👍 / 👎.
Promotes
inspect report— the lastinspectleaf still on the Phase 0 Go proxy — to a native TypeScript port in the legacy shell (CLI-1317). All 13 activeinspect dbsubcommands + 12 deprecated aliases were already native (#5554); this finishes theinspecttree.What it does
Runs every inspect query against the target Postgres database via server-side
COPY (<query>) TO STDOUT WITH CSV HEADER, writes one CSV per query into<output-dir>/<YYYY-MM-DD>/(14 files), then renders a Go-parity GlamourRULE | STATUS | MATCHESsummary table validating those CSVs.How it's built
copyToCsvon the shared driver (legacy-db-connection.sql-pg.layer.ts):@effect/sql-pgdoesn't expose the COPY protocol, so the session opens one rawpgconnection (viapg-copy-streams) lazily on the first copy and reuses it for all queries — against the same dial target the primary connection won (TLS/fallback/DoH/step-down parity preserved), and closed by a scope finalizer. This mirrors Go running every copy on a singlepgconn. CSVs are byte-identical by construction (the server serializes the values, never the TS side).report.csvq.ts): there is no JS port of csvq and neither DuckDB nor alasql accept its dialect, so the rule queries are evaluated by a hand-written tokenizer + recursive-descent parser + RFC4180 CSV reader that replicates csvq's value/type-comparison semantics (numeric-vs-string promotion, three-valued logic,LISTAGG, aggregates). Unsupported grammar / unknown column → the rule's STATUS cell, not a command failure (matching Go).[experimental.inspect.rules]fromconfig.toml(withenv(VAR)expansion) replace the 7 embedded defaults when present.inspect/inspect.layers.ts(now used by bothdbleaves andreport); addslegacy/output/legacy-bold.tsfor lipglossBoldparity.Reviewer context
s.tbl, whichvacuum_statsemits asname— a pre-existing quirk in Go'srules.tomlthat surfaces as an unknown-column STATUS cell. It's preserved verbatim (documented inreport.rules.ts+SIDE_EFFECTS.md); tests assert this Go-faithful behavior rather than "fixing" it.$2database literal is not escaped, matching Go'sfmt.Sprintf("'%s'", database).json/stream-jsonoutput modes are a TS-only addition (Go is text-only); CSVs are still written.pg+pg-copy-streamsare needed for the COPY protocol; verified to bundle underbun build --compile.--project-refflag that does not exist on the Goinspecttree — the surface is--db-url/--linked/--local/--output-dir, matching Go.Closes CLI-1317